Feature/request desktop site #198

CDRussell · 2018-02-22T15:41:44Z

Asana Issue URL: https://app.asana.com/0/488551667048375/529888388192278

Description

Gives the user an option to request that the desktop site be loaded instead of a mobile-specific site. It achieves this by altering the user agent string accordingly.

Note, if the url has an obviously mobile-specific site (using the m.xxxxxxxx.com) format, we strip the m. out of the URL when jumping to the desktop version. Discussions welcomed around this.

Also included in this is the slight improvement over the general UA strings we submit; they no longer contain the device make or model in them. We do still include the rest of the details, including the Android version and AppleWebKit/xxx.yy version information to ensure sites render well. https://app.asana.com/0/414730916066338/537987832233112

Steps to Test this PR:

Visit a site that redirects to a mobile-specific site (e.g., youtube.com)
Verify that you end up on the mobile site (m.youtube.com)
Use the overflow menu, and check to receive the desktop site
Verify that you end up on the desktop site
Repeat again to ensure you end up back on the mobile site

Includes removal of user device details - the make and model are omitted, though other details about the browser capabilities are retained to ensure a good browsing experience

…request_desktop_site # Conflicts: # app/src/main/res/layout/popup_window_browser_menu.xml

subsymbolic

Excellent work @CDRussell, I can't wait to start using this

subsymbolic · 2018-02-23T18:35:03Z

app/src/main/res/layout/popup_window_browser_menu.xml

                android:enabled="false"
                android:text="@string/find_in_page" />

+            <CheckBox


This doesn't look quite right visually. I'd suggest taking a screenshot and asking Mike to quickly mock it up in zeplin.

If you're keen to get the PR in right now and address design feedback later, I'd be happy to approve the PR if:

we update the text "Request Desktop Site" to "Desktop Site"? That's what other browsers use.

update the checkbox so it does not vertically overlap any of the menu items text (which it will after number one is done). Think of checkbox being is a different "column" to the text. Also, use the same margin/padding to the left and right of the menu. Right aligning the checkbox might give you all of this.

subsymbolic · 2018-02-23T18:48:22Z

app/src/androidTest/java/com/duckduckgo/app/browser/BrowserViewModelTest.kt

 import com.duckduckgo.app.browser.BrowserViewModel.Command.LandingPage
 import com.duckduckgo.app.browser.BrowserViewModel.Command.Navigate
 import com.duckduckgo.app.browser.omnibar.OmnibarEntryConverter
+import com.duckduckgo.app.browser.userAgent.UserAgentProvider


Can we use a lowercase package name here? https://kotlinlang.org/docs/reference/coding-conventions.html#naming-rules

subsymbolic · 2018-02-23T18:57:59Z

app/src/androidTest/java/com/duckduckgo/app/browser/BrowserViewModelTest.kt

+    @Test
+    fun whenUserSelectsDesktopSiteThenDesktopModeCommandIssued() {
+        testee.desktopSiteModeToggled("http://example.com", desktopSiteRequested = true)
+        verify(mockCommandObserver, Mockito.atLeastOnce()).onChanged(commandCaptor.capture())


Why is this command being triggered three times?

There are other commands that might already fire (like ShowKeyboard) so we don't want to verify only one command which is the default Mockito flow.

subsymbolic · 2018-02-23T19:03:01Z

app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt

        webView.webChromeClient = webChromeClient

        webView.settings.apply {
+            userAgentString = userAgentProvider.getUserAgent(defaultUserAgentString)


Optional: Would it make sense to instantiate the userAgentProvider with the defaultUserAgentString?

subsymbolic · 2018-02-23T19:12:56Z

app/src/main/java/com/duckduckgo/app/settings/db/SettingsDataStore.kt

    companion object {
-        val SHARED_PREFERENCES_FILENAME = "com.duckduckgo.app.settings_activity.settings"
-        val KEY_AUTOCOMPLETE_ENABLED = "AUTOCOMPLETE_ENABLED"
+        const val SHARED_PREFERENCES_FILENAME = "com.duckduckgo.app.settings_activity.settings"


I did the same thing in my branch :)

subsymbolic · 2018-02-23T19:29:29Z

app/src/main/java/com/duckduckgo/app/browser/userAgent/UserAgentProvider.kt

+
+fun Uri.isMobileSite() : Boolean = this.authority.startsWith("m.")
+
+fun Uri.toDesktopUri(): Uri {


These extensions feel generic rather than related to the user agent. Can we move them into UriExtensions, format and unit test them?

subsymbolic · 2018-02-23T19:34:43Z

app/src/main/java/com/duckduckgo/app/browser/userAgent/UserAgentProvider.kt

+fun Uri.isMobileSite() : Boolean = this.authority.startsWith("m.")
+
+fun Uri.toDesktopUri(): Uri {
+    return if(this.isMobileSite()) {


Do you need the explicit this?

Nope; removed

subsymbolic · 2018-02-23T19:37:04Z

app/src/main/java/com/duckduckgo/app/browser/userAgent/UserAgentProvider.kt

+
+fun Uri.toDesktopUri(): Uri {
+    return if(this.isMobileSite()) {
+        Uri.parse(toString().replaceFirst("m.", ""))


Can we protect future maintainers by adding a unit test for this to ensure that a non-mobile url with an ".m" e.g "adam.example.com" stays unchanged by this method?

Good thinking!

subsymbolic · 2018-02-23T19:39:43Z

app/src/main/java/com/duckduckgo/app/browser/userAgent/UserAgentProvider.kt

+ */
+class UserAgentProvider @Inject constructor() {
+
+    companion object {


Can we place companion objects at the bottom? I got the same feedback in my PR :) https://kotlinlang.org/docs/reference/coding-conventions.html#source-code-organization

subsymbolic · 2018-02-23T19:40:44Z

app/src/main/java/com/duckduckgo/app/browser/WebViewRequestInterceptor.kt

    }

+    private fun shouldStripRequestedWithHeader(request: WebResourceRequest): Boolean {
+        if(!request.isForMainFrame) return false


Spacing / formatting

Removed function entirely; was unused

CDRussell · 2018-02-26T12:43:04Z

RR @subsymbolic. all feedback should be addressed or, at least, given a response. let me know if i missed any.

subsymbolic

Looks great @CDRussell. One last bit of feedback, I'm seeing some odd progress bar behavior. To reproduce:

Go to facebook
Tap the checkbox to display a desktop site. All seems fine.
Tap the checkbox to display a mobile site. The progress bar finishes early and then there is a long delay before the mobile site is displayed which makes it seem like nothing is happening.

CDRussell · 2018-02-26T15:48:24Z

@subsymbolic I think you've unearthed something bigger.

It looks like BrowserWebViewClient.onPageFinished() is being called for every resource, including javascript ones. Therefore, it's calling through to BrowserViewModel.loadingFinished() which hides the loading indicator.

I think this is unrelated to the changes made for desktop mode but an important thing to fix. I will create a new ticket for this.

Internal reference: https://app.asana.com/0/414730916066338/572507223939055

subsymbolic · 2018-02-26T17:25:03Z

@CDRussell I think it is unrelated. I'm happy to forget that last feedback - testing it again I'm now inclined to think it was a facebook oddity as I managed to reproduce it on Chrome and our site. Subsequently, I couldn't reproduce it on either.

One other issue I noticed is that https://www.theguardian.com/uk stays on mobile irrespective of option. I wonder if viewport settings have something to do with this.

CDRussell · 2018-02-26T17:35:00Z

Let's class that problem as being sometimes the cause of what you saw, and other times something else is causing it. I'd like to address that ticket soon, but I think we're agreeing it's not needed for this PR. (For reference, loading gearbest.com is a good way to cause the symptoms you describe; the progress bar will disappear early.)

On the other hand, theguardian.com is a good problem site and should be solved in this PR. I'll investigate what's going on with this tomorrow.

…request_desktop_site # Conflicts: # app/src/main/java/com/duckduckgo/app/settings/db/SettingsDataStore.kt

CDRussell · 2018-02-27T17:05:27Z

Identified the problem with sites like theguardian.com is that because they have a responsive web page rather than a dedicated mobile site, the content of the web page is identical for both desktop and mobile UA strings.

Instead, we have to ensure we change some WebView settings locally based on which browsing mode we are in which will affect the scale on the WebView.

subsymbolic · 2018-02-27T17:40:09Z

This is really close. Portrait is looking great, landscape looks a little too "zoomed out". I have added screenshots of our app and chrome for comparison.

subsymbolic · 2018-02-27T17:49:13Z

Update: I see the same for portrait websites as well when in desktop mode. Facebook is a good one to test on as it happens in both portrait and landscape modes.

The result of this is that some responsive sites do not present differently under desktop or mobile modes. theguardian.com/uk is one such example site where we render both modes identically (despite Chrome rendering them correctly for each) Some other responsive sites do work just fine though - arstechnica.com for example renders differently for us under desktop and mobile modes. Will revisit fixing sites like theguardian.com/uk in another task.

…request_desktop_site

subsymbolic

Just one comment about mobile options.

subsymbolic · 2018-02-28T13:57:42Z

app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt

            javaScriptEnabled = true
            domStorageEnabled = true
-            loadWithOverviewMode = true
-            useWideViewPort = true


Why remove these two options from mobile? Overview mode was introduced to fix an issue #70.

CDRussell · 2018-02-28T14:50:54Z

The result of these changes is that some responsive sites do not present differently under desktop or mobile modes. theguardian.com/uk is one such example site where we render both modes identically (despite Chrome rendering them correctly for each)

Some other responsive sites do work just fine though - arstechnica.com for example renders differently for us under desktop and mobile modes.

Will revisit fixing sites like theguardian.com/uk in another task.

subsymbolic

Looks good!

…ity is destroyed (#198)

CDRussell added 3 commits February 21, 2018 13:29

First pass at "request desktop site" mode.

5d1c412

Includes removal of user device details - the make and model are omitted, though other details about the browser capabilities are retained to ensure a good browsing experience

Tidy up request desktop site feature

e5ea895

Fix last few failing tests after refactor

dc09e26

CDRussell requested review from brindy and subsymbolic February 22, 2018 15:41

CDRussell mentioned this pull request Feb 22, 2018

[Feature Request] See Desktop version of websites #199

Closed

subsymbolic self-assigned this Feb 23, 2018

CDRussell mentioned this pull request Feb 23, 2018

Fix landscape layout's squished controls; add scroll view to menu items #197

Merged

Merge branch 'develop' of github.com:duckduckgo/Android into feature/…

7c5f687

…request_desktop_site # Conflicts: # app/src/main/res/layout/popup_window_browser_menu.xml

subsymbolic reviewed Feb 23, 2018

View reviewed changes

CDRussell added 6 commits February 26, 2018 12:01

Change string to "Desktop Site" instead of "Request Desktop Site"

baaae01

Make "userAgent" package lowercase

980a36d

Fix checkbox UI - tweaking paddings and margins

ae1c31e

Instantiate UserAgentProvider with default user agent string

5786279

Move url checking around desktop/mobile site to UriExtension.kt

f4b9124

Remove unused function

aa05e11

subsymbolic reviewed Feb 26, 2018

View reviewed changes

CDRussell added 2 commits February 27, 2018 16:55

Add better support for responsive websites when toggling desktop mode

d318565

Merge branch 'develop' of github.com:duckduckgo/Android into feature/…

3e4204e

…request_desktop_site # Conflicts: # app/src/main/java/com/duckduckgo/app/settings/db/SettingsDataStore.kt

CDRussell added 3 commits February 28, 2018 12:31

Tweak initial scale value up to 120

8861108

Merge branch 'develop' of github.com:duckduckgo/Android into feature/…

ffef1ca

…request_desktop_site

subsymbolic reviewed Feb 28, 2018

View reviewed changes

Return loadWithOverviewMode and useWideViewPort to always be true

aa2ad28

subsymbolic approved these changes Feb 28, 2018

View reviewed changes

CDRussell merged commit b18948e into develop Feb 28, 2018

CDRussell deleted the feature/request_desktop_site branch February 28, 2018 16:07

aitorvs pushed a commit that referenced this pull request Nov 18, 2021

FIX vpn not restarting back when TrackingProtectionExclusionListActiv…

7b79bd9

…ity is destroyed (#198)


		fun Uri.isMobileSite() : Boolean = this.authority.startsWith("m.")

		fun Uri.toDesktopUri(): Uri {

Feature/request desktop site #198

Feature/request desktop site #198

Uh oh!

Conversation

CDRussell commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Steps to Test this PR:

Uh oh!

subsymbolic left a comment

Choose a reason for hiding this comment

Uh oh!

subsymbolic Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CDRussell commented Feb 26, 2018

Uh oh!

subsymbolic left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CDRussell commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

subsymbolic commented Feb 26, 2018

Uh oh!

CDRussell commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CDRussell commented Feb 27, 2018

Uh oh!

subsymbolic commented Feb 27, 2018

Uh oh!

subsymbolic commented Feb 27, 2018

Uh oh!

subsymbolic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CDRussell commented Feb 28, 2018

Uh oh!

subsymbolic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

CDRussell commented Feb 22, 2018 •

edited

Loading

subsymbolic Feb 23, 2018 •

edited

Loading

subsymbolic left a comment •

edited

Loading

CDRussell commented Feb 26, 2018 •

edited

Loading

CDRussell commented Feb 26, 2018 •

edited

Loading