Tab grid layout failure

[Outi-s]

Bromite version

Version: v79.0.3945.94
Arch: arm
Android version: 8.1
Device model: Moto XT1804 (G5S+)

Is this bug about the SystemWebView?

No

Is the bug reproducible with latest version?

This behaviour only appears in the latest build version.

Can the bug be reproduced with corresponding Chromium version?

If this means the same kind of thing happens on Chrome, then no.

Is the bug a crash?

No.

Describe the bug

I have #enable-tab-grid-layout enabled but it does not seem to work. I have restarted it twice, yes.

Steps to reproduce the bug

1. Enable the flag.
2. Restart twice.

Expected behavior

Unable to see the tabs in a grid layout in the tab switcher.

[nikolowry]

+1. Had been using the feature successfully in past versions.

Couldn't test against the vanilla Chromium builds as bromite.org's download link is returning a 404, https://github.com/bromite/chromium/releases/download/79.0.3945.94/chr_arm64_ChromeModernPublic.apk

I'll be looking into the cause of the regression this weekend.

[nikolowry]

The enable-tab-grid-layout regression was due to the newly introduced GN arg disable_tab_ui_dfm=false.

Being an F-Droid user, this is my first encounter with Dynamic Feature Modules.

What I believe is happening, is when that GN arg was set to false it translates to "Enable TabUI Dynamic Feature Modules". Since we killed Dynamic Feature Modules with Disable-dynamic-module-loading.patch the enable-tab-grid-layout flag fails as it should.

I just rebuilt with disable_tab_ui_dfm=true and as expected the compilation steps increased from ~25000 to 29348. The newly built apk has also increased in size from 99.1MB to 99.2MB.

For the build to succeed, I had to drop code from two additional files -- which at a glance seems to be Feed related functionality. I'm uncertain if more privacy related changes are needed for the new Tabs UI feature, but here's the patch I applied:

diff -rupN a/chrome/android/features/start_surface/internal/java/src/org/chromium/chrome/features/start_surface/StartSurfaceCoordinator.java b/chrome/android/features/start_surface/internal/java/src/org/chromium/chrome/features/start_surface/StartSurfaceCoordinator.java
--- a/chrome/android/features/start_surface/internal/java/src/org/chromium/chrome/features/start_surface/StartSurfaceCoordinator.java   2019-12-20 22:33:31.087038599 -0500
+++ b/chrome/android/features/start_surface/internal/java/src/org/chromium/chrome/features/start_surface/StartSurfaceCoordinator.java   2019-12-20 22:36:08.291417393 -0500
@@ -198,11 +198,6 @@ public class StartSurfaceCoordinator imp
             mBottomBarCoordinator = new BottomBarCoordinator(
                     mActivity, mActivity.getCompositorViewHolder(), mPropertyModel);
         }
-
-        mExploreSurfaceCoordinator = new ExploreSurfaceCoordinator(mActivity,
-                mSurfaceMode == SurfaceMode.SINGLE_PANE ? mTasksSurface.getBodyViewContainer()
-                                                        : mActivity.getCompositorViewHolder(),
-                mPropertyModel, mSurfaceMode == SurfaceMode.SINGLE_PANE);
     }
 
     private TabSwitcher.Controller initializeSecondaryTasksSurface() {
diff -rupN a/chrome/android/features/start_surface/internal/java/src/org/chromium/chrome/features/start_surface/StartSurfaceMediator.java b/chrome/android/features/start_surface/internal/java/src/org/chromium/chrome/features/start_surface/StartSurfaceMediator.java
--- a/chrome/android/features/start_surface/internal/java/src/org/chromium/chrome/features/start_surface/StartSurfaceMediator.java  2019-12-20 22:33:46.223807608 -0500
+++ b/chrome/android/features/start_surface/internal/java/src/org/chromium/chrome/features/start_surface/StartSurfaceMediator.java  2019-12-20 22:37:30.241949130 -0500
@@ -253,15 +253,6 @@ class StartSurfaceMediator
                 RecordUserAction.record("StartSurface.TasksOnly");
             }
 
-            // Make sure FeedSurfaceCoordinator is built before the explore surface is showing by
-            // default.
-            if (mPropertyModel.get(IS_EXPLORE_SURFACE_VISIBLE)
-                    && mPropertyModel.get(FEED_SURFACE_COORDINATOR) == null) {
-                mPropertyModel.set(FEED_SURFACE_COORDINATOR,
-                        mFeedSurfaceCreator.createFeedSurfaceCoordinator(
-                                mNightModeStateProvider.isInNightMode()));
-            }
-
             mPropertyModel.set(IS_SHOWING_OVERVIEW, true);
             mFakeboxDelegate.addUrlFocusChangeListener(mUrlFocusChangeListener);
         }
@@ -361,13 +352,6 @@ class StartSurfaceMediator
     private void setExploreSurfaceVisibility(boolean isVisible) {
         if (isVisible == mPropertyModel.get(IS_EXPLORE_SURFACE_VISIBLE)) return;
 
-        if (isVisible && mPropertyModel.get(IS_SHOWING_OVERVIEW)
-                && mPropertyModel.get(FEED_SURFACE_COORDINATOR) == null) {
-            mPropertyModel.set(FEED_SURFACE_COORDINATOR,
-                    mFeedSurfaceCreator.createFeedSurfaceCoordinator(
-                            mNightModeStateProvider.isInNightMode()));
-        }
-
         mPropertyModel.set(IS_EXPLORE_SURFACE_VISIBLE, isVisible);
 
         if (mSurfaceMode == SurfaceMode.TWO_PANES) {

Screenshot of new Tab UI working:

Please note this also applies to the new disable_autofill_assistant_dfm=false arg as well. Tested password autofilling and a few form input autofilling and they were still working correctly in is this build -- so I'm not really sure what "Autofill Assistant" provides.

---

Off-topic

• The duplicate symbol_level was fixed, but now blink_symbol_level is set twice:

  bromite/build/GN_ARGS

  Lines 2 to 3 in 6340bb4

  	blink_symbol_level=0
  	blink_symbol_level=1

• Builds using chromium_patches_list.txt are failing due to two failing patches. Here's stdout of when applying those patches:

Applying patch: Revert-Remove-pre-unified-consent-code-in-sync-and-privacy-directory.patch
Applying patch: Disable-unified-consent-on-Android.patch
Skipping, doesn't apply: Unified-consent-miscellanous-backport-fixes.patch
Applying patch: AV1-codec-support.patch
Applying patch: Switch-to-fstack-protector-strong.patch
Applying patch: Enable-fwrapv-in-Clang-for-non-UBSan-builds.patch
Skipping, doesn't apply: Disable-password-reuse-detection-on-android.patch

Here's the build error:

[18711/27614] CXX obj/chrome/common/safe_browsing/file_type_policies/file_type_policies.o
FAILED: obj/chrome/common/safe_browsing/file_type_policies/file_type_policies.o 
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/chrome/common/safe_browsing/file_type_policies/file_type_policies.o.d -DOFFICIAL_BUILD -DNO_UNWIND_TABLES -D_GNU_SOURCE -DANDROID -DHAVE_SYS_UIO_H -DANDROID_NDK_VERSION_ROLL=r20_1 -DCR_CLANG_REVISION=\"373424-64a362e7-1\" -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -D_LIBCPP_ABI_UNSTABLE -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_ENABLE_NODISCARD -DCR_LIBCXX_REVISION=361348 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DWEBP_EXTERN=extern -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DHAVE_PTHREAD -DSK_GL -DSK_HAS_PNG_LIBRARY -DSK_HAS_WEBP_LIBRARY -DSK_USER_CONFIG_HEADER=\"../../skia/config/SkUserConfig.h\" -DSK_HAS_JPEG_LIBRARY -DSK_VULKAN_HEADER=\"../../skia/config/SkVulkanConfig.h\" -DSK_VULKAN=1 -DSK_SUPPORT_GPU=1 -DSK_GPU_WORKAROUNDS_HEADER=\"gpu/config/gpu_driver_bug_workaround_autogen.h\" -DSK_BUILD_FOR_ANDROID -DUSE_CHROMIUM_SKIA -DVK_NO_PROTOTYPES -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DUSE_CHROMIUM_ICU=1 -DU_STATIC_IMPLEMENTATION -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DUCHAR_TYPE=uint16_t -I../.. -Igen -I../../third_party/libwebp/src -I../../third_party/protobuf/src -Igen/protoc_out -I../../third_party/protobuf/src -I../../third_party/boringssl/src/include -I../../third_party/skia -I../../third_party/vulkan/include -I../../third_party/skia/third_party/vulkanmemoryallocator -I../../third_party/vulkan/include -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -fprofile-sample-use=../../chrome/android/profiles/afdo.prof -fprofile-sample-accurate -fno-strict-aliasing -fstack-protector-strong -fwrapv -fno-unwind-tables -fno-asynchronous-unwind-tables -fPIC -fcolor-diagnostics -fmerge-all-constants -fcrash-diagnostics-dir=../../tools/clang/crashreports -Xclang -mllvm -Xclang -instcombine-lower-dbg-declare=0 -flto=thin -fsplit-lto-unit -fcomplete-member-pointers -ffunction-sections -fno-short-enums --target=aarch64-linux-android21 -mno-outline -no-canonical-prefixes -Wall -Werror -Wextra -Wimplicit-fallthrough -Wthread-safety -Wextra-semi -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-unneeded-internal-declaration -Wno-undefined-var-template -Wno-ignored-pragma-optimize -Wno-implicit-int-float-conversion -Wno-c99-designator -Wno-reorder-init-list -Wno-final-dtor-non-final-class -Wno-sizeof-array-div -Oz -fno-ident -fdata-sections -ffunction-sections -fno-omit-frame-pointer -g0 -fsanitize=cfi-vcall -fsanitize-blacklist=../../tools/cfi/blacklist.txt -fvisibility=hidden -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang check-ipc -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -Wno-shorten-64-to-32 -std=c++14 -fno-exceptions -fno-rtti -nostdinc++ -isystem../../buildtools/third_party/libc++/trunk/include -isystem../../buildtools/third_party/libc++abi/trunk/include --sysroot=../../third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot -fvisibility-inlines-hidden -c ../../chrome/common/safe_browsing/file_type_policies.cc -o obj/chrome/common/safe_browsing/file_type_policies/file_type_policies.o
../../chrome/common/safe_browsing/file_type_policies.cc:53:29:  

error: use of undeclared identifier 'IDR_DOWNLOAD_FILE_TYPES_PB'
  bundle.GetRawDataResource(IDR_DOWNLOAD_FILE_TYPES_PB).CopyToString(binary_pb);

[csagan5]

Can the bug be reproduced with corresponding Chromium version?

If this means the same kind of thing happens on Chrome, then no.

No, it means what is written in the template:

Please pick the same version of Chromium as Bromite from here: https://github.com/bromite/chromium/releases If the bug is reproducible then it might be a configuration issue or an upstream bug. Upstream bugs can be reported on the Chromium issue tracker and do not forget to read Chromium project bug reporting guidelines first.

[csagan5]

• Builds using chromium_patches_list.txt are failing due to two failing patches. Here's stdout of when applying those patches:

@nikolowry yes, I also experienced that failure and there was a patch for it in the first commit of the v79 branch.
I also have to adjust the patches for Chromium (that is why I did not release the Chromium APKs yet).

Re-opening because currently issue cannot be tested with Chromium.

The enable-tab-grid-layout regression was due to the newly introduced GN arg disable_tab_ui_dfm=false.

Being an F-Droid user, this is my first encounter with Dynamic Feature Modules.

I also new to DFM and modules in general, but their increased usage in latest v79 does not give me a good vibe: from what I could see they increase Play integrations in the codebase, and that was my rationale to configure the flags and use the DFM patch that way.

@nikolowry since in Bromite I am trying to not have any binary binding with Play integrations, would you say that disabling the DFM flag for those options will still allow building without Play integrations? If yes then we can change them back to be enabled and gain the missing functionality reported here.

[csagan5]

For the build to succeed, I had to drop code from two additional files -- which at a glance seems to be Feed related functionality.

I already added a patch to allow building with enable_feed_in_chrome=false, so I am not surprised. Seems like disabling this flag is not tested by upstream.

I'm not really sure what "Autofill Assistant" provides.

It's functionality to autofill addresses etc.; it needs cloud integrations (via Play Core library) so not supported in Bromite anyways.

• The duplicate symbol_level was fixed, but now blink_symbol_level is set twice:

Only the latest one is used, so this has no functional effect (only confuses the reader).

Edit: @nikolowry I will fix the patches for Chromium in #447

Good catch for the relevance of DFM on this issue, I will try also to build with the inverted DFM flags, although let me know about what you have observed when building re Play integrations (I assume it built fine for you with the Bromite patches which disable them).

[csagan5]

Issue has been fixed in 79.0.3945.100, please notice you have to restart it twice after changing the flag.

[Outi-s]

Thank you @csagan5, works fine now.

[nikolowry]

@csagan5 sorry for the delay, got caught up in holiday-shenanigans. Glad you were able to get this resolved!

There was one more thing I wanted to bring up though, the enable-tab-grid-layout flag is set to expire at v82. I have a feeling they'll be adopting the DFM pattern for future experiments as well, which means you'll need to be on the look-out to make judgement calls whether it'll be "worth it or not" to support such flags.

I do hope they make the tab-grid layout the default though, it was one of the few things Firefox on Android does better than Chromium.

[csagan5]

you'll need to be on the look-out to make judgement calls whether it'll be "worth it or not" to support such flags.

The decision to not support DFM is based on the fact that modules need the Play Store integrations (correct me if I am wrong), thus the decision is very simple: they are out.

The Play Store blobs have been removed much time ago and re-introducing them would mean to "cave in" to binary/opaque code into the browser APK, not something I am willing to do. You can look into microG for a replacement if you like, but from my understanding we are waay far from anything usable (plus did not have a great experience interacting with them some time ago).

[nikolowry]

you'll need to be on the look-out to make judgement calls whether it'll be "worth it or not" to support such flags.

The decision to not support DFM is based on the fact that modules need the Play Store integrations (correct me if I am wrong), thus the decision is very simple: they are out.

The Play Store blobs have been removed much time ago and re-introducing them would mean to "cave in" to binary/opaque code into the browser APK, not something I am willing to do. You can look into microG for a replacement if you like, but from my understanding we are waay far from anything usable (plus did not have a great experience interacting with them some time ago).

I didn't articulate properly, what I meant was writing extra patches to de-DFM a feature like this issue.

Regardless, thanks for the continued work @csagan5!

[csagan5]

@nikolowry if possible without too many files changes then yeah we could have patches for it, otherwise it's unlikely.

I hope that a feature like this (allowing apps to expose switchable tabs at OS level) will be part of the Android OS rather than Play Store, it is not the current direction though.

[Outi-s]

Hey @csagan5, is DFM the reason for notifications failure as well? Bromite shows Notifications: Allowed on site but it doesn't work. Example site: spacebattles.

Also, Happy New Year to all! 🎉

[Nemris]

@csagan5

I'm noticing a decidedly weird behavior on the latest Bromite build, when ran on a Samsung Galaxy S3 with LineageOS 16 (Pie).

Basically, the tab thumbnails never get displayed, and changing #enable-tab-grid-layout changes absolutely nothing. The thumbnails are displayed correctly with a different LOS version, or on a different device running the same LOS version, the APK being the same one.

Moreover, on the S3 running Pie, the swiping gestures to view the tab grib and to switch to the previous or next tab aren't working.

Is this intended behavior?