[Vertical Tabs] Add Glic promo IPH to tablet toolbar action chip [chromium/src : main]
Gazal Agarwal (Gerrit)
Gazal Agarwal voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
- Jinsuk Kim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jinsuk Kim (Gerrit)
Jinsuk Kim added 4 comments![]( "Open in Gerrit")
Thanks for handling this. Adding mklw@ for another pair of eyes on Glic IPH condition.
mEducationHelper.requestShowIph(I think the cadence/frequency defined for Glic IPH when shown in TabStrip should be also applied here when it is shown as toolbar button. For instance, if we have shown IPH enough for TabStrip, then no need to show it for GLIC as toolbar button either. @mk...@google.com would you help review this? I wonder if this should call wouldTriggerIph or something equivalent here https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java;l=1735;drc=38608b7776ceea9eb2e9a1a138d107b38ad9cf68
View anchorView = mToolbar.getGlicActionChipView();At this point anchorView cannot be null after all the checks above, I think. assumeNonNull?
return mIsVerticalTabsActiveSupplier != nullYou may also use |mToolbarLayou instanceof... | to save many null checks (see line #1219)
Related details
- Gazal Agarwal
- Michael Wu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gazal Agarwal (Gerrit)
Gazal Agarwal added 3 comments![]( "Open in Gerrit")
mEducationHelper.requestShowIph(I think the cadence/frequency defined for Glic IPH when shown in TabStrip should be also applied here when it is shown as toolbar button. For instance, if we have shown IPH enough for TabStrip, then no need to show it for GLIC as toolbar button either. @mk...@google.com would you help review this? I wonder if this should call wouldTriggerIph or something equivalent here https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java;l=1735;drc=38608b7776ceea9eb2e9a1a138d107b38ad9cf68
`requestShowIph()` calls `tracker.shouldTriggerHelpUi()` under the hood. Because both TabStrip and Toolbar share FeatureConstants.GLIC_PROMO_ANDROID_FEATURE, Tracker already enforces shared cadence and frequency caps across both surfaces.
We can add an explicit wouldTriggerHelpUi pre-check as well to ToolbarIphController. That would require passing a `Supplier<Tracker>` from ToolbarManager into ToolbarIphController
At this point anchorView cannot be null after all the checks above, I think. assumeNonNull?
Done
You may also use |mToolbarLayou instanceof... | to save many null checks (see line #1219)
- Jinsuk Kim
- Michael Wu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jinsuk Kim (Gerrit)
Jinsuk Kim voted and added 4 comments![]( "Open in Gerrit")
Votes added by Jinsuk Kim
| Code-Review | +1 |
4 comments
mEducationHelper.requestShowIph(Gazal AgarwalI think the cadence/frequency defined for Glic IPH when shown in TabStrip should be also applied here when it is shown as toolbar button. For instance, if we have shown IPH enough for TabStrip, then no need to show it for GLIC as toolbar button either. @mk...@google.com would you help review this? I wonder if this should call wouldTriggerIph or something equivalent here https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java;l=1735;drc=38608b7776ceea9eb2e9a1a138d107b38ad9cf68
`requestShowIph()` calls `tracker.shouldTriggerHelpUi()` under the hood. Because both TabStrip and Toolbar share FeatureConstants.GLIC_PROMO_ANDROID_FEATURE, Tracker already enforces shared cadence and frequency caps across both surfaces.
We can add an explicit wouldTriggerHelpUi pre-check as well to ToolbarIphController. That would require passing a `Supplier<Tracker>` from ToolbarManager into ToolbarIphController
Acknowledged
&& mIsVerticalTabsActiveSupplier != nullThis may be removed. It should have a non-null supplier assigned by this time.
&& Boolean.TRUE.equals(mIsVerticalTabsActiveSupplier.get())
&& Boolean.TRUE.equals(mIsGlicPinnedSupplier.get())These two can be simplied to mIsXXX.get()
Related details
- Gazal Agarwal
- Michael Wu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gazal Agarwal (Gerrit)
Gazal Agarwal added 2 comments![]( "Open in Gerrit")
This may be removed. It should have a non-null supplier assigned by this time.
Done. Updated to use assumeNonNull(...) to remove the explicit null check while satisfying NullAway.
&& Boolean.TRUE.equals(mIsVerticalTabsActiveSupplier.get())
&& Boolean.TRUE.equals(mIsGlicPinnedSupplier.get())These two can be simplied to mIsXXX.get()
- Michael Wu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wu (Gerrit)
Michael Wu voted and added 2 comments![]( "Open in Gerrit")
Votes added by Michael Wu
| Code-Review | +1 |
2 comments
assertEquals(10000L, cmd.autoDismissTimeout);I think this is set to 5s in ToolbarIphController
Related details
- Gazal Agarwal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wu (Gerrit)
Michael Wu added 1 comment![]( "Open in Gerrit")
public boolean shouldShowGlicToolbarButton() {nit: docstring
Gazal Agarwal (Gerrit)
Gazal Agarwal voted and added 4 comments![]( "Open in Gerrit")
Votes added by Gazal Agarwal
| Commit-Queue | +2 |
4 comments
I think this is set to 5s in ToolbarIphController
That's right, updated.
public boolean shouldShowGlicToolbarButton() {Gazal Agarwalnit: docstring
Done
&& mIsVerticalTabsActiveSupplier != nullGazal AgarwalThis may be removed. It should have a non-null supplier assigned by this time.
Done. Updated to use assumeNonNull(...) to remove the explicit null check while satisfying NullAway.
Added back the null checks because integration tests (e.g. ActorNotificationClickIntegrationTest) failed with a NullPointerException. The suppliers are uninitialized before observeGlicVerticalTabs() runs during startup and remain null on phones.
Related details
- Theresa Wellington
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jinsuk Kim (Gerrit)
Jinsuk Kim voted![]( "Open in Gerrit")
- Gazal Agarwal
- Michael Wu
- Theresa Wellington
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Theresa Wellington (Gerrit)
Theresa Wellington voted![]( "Open in Gerrit")
| Code-Review | +1 |
| Commit-Queue | +2 |
Related details
- Gazal Agarwal
- Jinsuk Kim
- Michael Wu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Vertical Tabs] Add Glic promo IPH to tablet toolbar action chip
When Vertical Tabs is active, the Glic button is displayed on the
toolbar, instead of the horizontal tab strip. When Glic is active, users
should be presented with an in-product help (IPH) promo bubble anchored
to the Glic toolbar action chip.
This CL Triggers the Glic promo IPH from ToolbarManager on tab selection
and page load completion when the tab and toolbar button are eligible.
Horizontal Tabs:
https://screenshot.googleplex.com/image/Bs8rJFxR8QAPJ5P.png
Vertical Tabs: https://screenshot.googleplex.com/image/9raJra2ySYVnuse.png
- M chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarIphController.java
- M chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
- M chrome/android/junit/BUILD.gn
- A chrome/android/junit/src/org/chromium/chrome/browser/toolbar/ToolbarIphControllerUnitTest.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/ToolbarTablet.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/TopToolbarCoordinator.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/TopToolbarCoordinatorUnitTest.java
- M components/feature_engagement/public/feature_list.h
Code-Review: +1 by Jinsuk Kim, +1 by Theresa Wellington
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |