Consider moving IPH from Tab to feature-specific classes |
||||
Issue descriptionThis comes from a comment in https://chromium-review.googlesource.com/c/chromium/src/+/1234637 There's a lot of event notifications and IPH code in Tab.didFinishPageLoad which could be moved to a user data on tab like InfobarContainer.
,
Oct 2
twellington@ had some thoughts around this as well (they might not necessarily fit as user data if they don't need to have any state on a per-tab basis). e.g. we could also just be one of those other observers that always watches the current tab displayed to the user. +some people who might have input.
,
Oct 10
Working on it now.. looks like TabObserver should be enough for this.
,
Nov 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/41ee7d38d589f07b27671455fff28041ca32aee7 commit 41ee7d38d589f07b27671455fff28041ca32aee7 Author: Jinsuk Kim <jinsukkim@chromium.org> Date: Wed Nov 21 01:25:13 2018 Android: Factor InProductHelp UI out of Tab This CL moves the methods handling InProductHelp UI in Tab to ToolbarButtonInProductHelpController using ActivityLifecycleDispatcher and Tab page load observing mechanism. Now the methods are handled by ChromeTabbedActivity only. Bug: 889682 Change-Id: Idce805f60f32ad90418c0f6e3ded80df6d18f9b1 Reviewed-on: https://chromium-review.googlesource.com/c/1272779 Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org> Reviewed-by: Ted Choc <tedchoc@chromium.org> Reviewed-by: Theresa <twellington@chromium.org> Cr-Commit-Position: refs/heads/master@{#609881} [modify] https://crrev.com/41ee7d38d589f07b27671455fff28041ca32aee7/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java [modify] https://crrev.com/41ee7d38d589f07b27671455fff28041ca32aee7/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java [modify] https://crrev.com/41ee7d38d589f07b27671455fff28041ca32aee7/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarButtonInProductHelpController.java
,
Nov 21
,
Nov 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d436a7a2e33ee105a1d85370970fde3c88255302 commit d436a7a2e33ee105a1d85370970fde3c88255302 Author: Brian Sheedy <bsheedy@chromium.org> Date: Wed Nov 21 23:21:42 2018 Revert "Android: Factor InProductHelp UI out of Tab" This reverts commit 41ee7d38d589f07b27671455fff28041ca32aee7. Reason for revert: Cause of https://crbug.com/907602 Original change's description: > Android: Factor InProductHelp UI out of Tab > > This CL moves the methods handling InProductHelp UI in Tab to > ToolbarButtonInProductHelpController using ActivityLifecycleDispatcher and > Tab page load observing mechanism. Now the methods are handled by > ChromeTabbedActivity only. > > Bug: 889682 > Change-Id: Idce805f60f32ad90418c0f6e3ded80df6d18f9b1 > Reviewed-on: https://chromium-review.googlesource.com/c/1272779 > Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org> > Reviewed-by: Ted Choc <tedchoc@chromium.org> > Reviewed-by: Theresa <twellington@chromium.org> > Cr-Commit-Position: refs/heads/master@{#609881} TBR=tedchoc@chromium.org,twellington@chromium.org,mdjones@chromium.org,jinsukkim@chromium.org,robertogden@chromium.org Change-Id: I539d82b07ee8f6a3012ad72e1f2ba35eda613d72 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 889682 Reviewed-on: https://chromium-review.googlesource.com/c/1347199 Reviewed-by: Brian Sheedy <bsheedy@chromium.org> Commit-Queue: Brian Sheedy <bsheedy@chromium.org> Cr-Commit-Position: refs/heads/master@{#610258} [modify] https://crrev.com/d436a7a2e33ee105a1d85370970fde3c88255302/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java [modify] https://crrev.com/d436a7a2e33ee105a1d85370970fde3c88255302/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java [modify] https://crrev.com/d436a7a2e33ee105a1d85370970fde3c88255302/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarButtonInProductHelpController.java
,
Dec 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/76d39b3a8d8fbd0e990f92a992d80213724de9bd commit 76d39b3a8d8fbd0e990f92a992d80213724de9bd Author: Jinsuk Kim <jinsukkim@chromium.org> Date: Wed Dec 05 23:06:59 2018 Reland "Android: Factor InProductHelp UI out of Tab" https://chromium-review.googlesource.com/c/chromium/src/+/1272779 had a couple of bugs in |ToolbarButtonInProductHelpController|: 1) Ignored |profile| passed into |maybeShowDownloadContinuingIPH()| and always used Profile.getLastUsedProfile() 2) |OnDismissListener| was instantiated (and menu got highlight for the text bubble) every time |setupAndMaybeShowIPHForFeature| is called, even when the method doesn't meet the condition for IPH text bubble and has to early out. The CL got reverted due to numerous regressions . This CL fixes them and relands it. This reverts commit d436a7a2e33ee105a1d85370970fde3c88255302. Bug: 889682, 907502, 907503, 907770, 907505, 907602 Change-Id: Id6de148a9fd4fd91ad7df986ca0ba78ed18e79e4 Reviewed-on: https://chromium-review.googlesource.com/c/1349142 Reviewed-by: Theresa <twellington@chromium.org> Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org> Cr-Commit-Position: refs/heads/master@{#614161} [modify] https://crrev.com/76d39b3a8d8fbd0e990f92a992d80213724de9bd/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java [modify] https://crrev.com/76d39b3a8d8fbd0e990f92a992d80213724de9bd/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java [modify] https://crrev.com/76d39b3a8d8fbd0e990f92a992d80213724de9bd/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarButtonInProductHelpController.java
,
Dec 6
Repurposed this bug for tracking the refactoring effort around IPH.
,
Dec 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fbe75ef510a9d9123f6c7f8cbd54d5f5b5680277 commit fbe75ef510a9d9123f6c7f8cbd54d5f5b5680277 Author: Jinsuk Kim <jinsukkim@chromium.org> Date: Thu Dec 06 22:10:37 2018 Revert "Reland "Android: Factor InProductHelp UI out of Tab"" This reverts commit 76d39b3a8d8fbd0e990f92a992d80213724de9bd. Reason for revert: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Oreo%20Phone%20Tester/2097 https://crbug.com/907602 doesn't seem to be addressed. Original change's description: > Reland "Android: Factor InProductHelp UI out of Tab" > > > https://chromium-review.googlesource.com/c/chromium/src/+/1272779 had a couple > of bugs in |ToolbarButtonInProductHelpController|: > > 1) Ignored |profile| passed into |maybeShowDownloadContinuingIPH()| and > always used Profile.getLastUsedProfile() > 2) |OnDismissListener| was instantiated (and menu got highlight for > the text bubble) every time |setupAndMaybeShowIPHForFeature| is called, > even when the method doesn't meet the condition for IPH text bubble > and has to early out. > > The CL got reverted due to numerous regressions . This CL fixes them and > relands it. > > This reverts commit d436a7a2e33ee105a1d85370970fde3c88255302. > > Bug: 889682, 907502, 907503, 907770, 907505, 907602 > Change-Id: Id6de148a9fd4fd91ad7df986ca0ba78ed18e79e4 > Reviewed-on: https://chromium-review.googlesource.com/c/1349142 > Reviewed-by: Theresa <twellington@chromium.org> > Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org> > Cr-Commit-Position: refs/heads/master@{#614161} TBR=twellington@chromium.org,jinsukkim@chromium.org Change-Id: Id92d47e6ac65a0b1a50f4ad4cb1c0b9a11189993 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 889682, 907502, 907503, 907770, 907505, 907602 Reviewed-on: https://chromium-review.googlesource.com/c/1366456 Reviewed-by: Jinsuk Kim <jinsukkim@chromium.org> Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org> Cr-Commit-Position: refs/heads/master@{#614501} [modify] https://crrev.com/fbe75ef510a9d9123f6c7f8cbd54d5f5b5680277/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java [modify] https://crrev.com/fbe75ef510a9d9123f6c7f8cbd54d5f5b5680277/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java [modify] https://crrev.com/fbe75ef510a9d9123f6c7f8cbd54d5f5b5680277/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarButtonInProductHelpController.java
,
Dec 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b643010d742b1323893685b3ded2250d684882c4 commit b643010d742b1323893685b3ded2250d684882c4 Author: Jinsuk Kim <jinsukkim@chromium.org> Date: Sat Dec 08 00:06:02 2018 Reland "Android: Factor InProductHelp UI out of Tab" again This reverts commit fbe75ef510a9d9123f6c7f8cbd54d5f5b5680277. Reason for revert: Reverted on a false alarm. Landing again. Original change's description: > Revert "Reland "Android: Factor InProductHelp UI out of Tab"" > > This reverts commit 76d39b3a8d8fbd0e990f92a992d80213724de9bd. > > Reason for revert: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Oreo%20Phone%20Tester/2097 > > https://crbug.com/907602 doesn't seem to be addressed. > > Original change's description: > > Reland "Android: Factor InProductHelp UI out of Tab" > > > > > > https://chromium-review.googlesource.com/c/chromium/src/+/1272779 had a couple > > of bugs in |ToolbarButtonInProductHelpController|: > > > > 1) Ignored |profile| passed into |maybeShowDownloadContinuingIPH()| and > > always used Profile.getLastUsedProfile() > > 2) |OnDismissListener| was instantiated (and menu got highlight for > > the text bubble) every time |setupAndMaybeShowIPHForFeature| is called, > > even when the method doesn't meet the condition for IPH text bubble > > and has to early out. > > > > The CL got reverted due to numerous regressions . This CL fixes them and > > relands it. > > > > This reverts commit d436a7a2e33ee105a1d85370970fde3c88255302. > > > > Bug: 889682, 907502, 907503, 907770, 907505, 907602 > > Change-Id: Id6de148a9fd4fd91ad7df986ca0ba78ed18e79e4 > > Reviewed-on: https://chromium-review.googlesource.com/c/1349142 > > Reviewed-by: Theresa <twellington@chromium.org> > > Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#614161} > > TBR=twellington@chromium.org,jinsukkim@chromium.org > > Change-Id: Id92d47e6ac65a0b1a50f4ad4cb1c0b9a11189993 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: 889682, 907502, 907503, 907770, 907505, 907602 > Reviewed-on: https://chromium-review.googlesource.com/c/1366456 > Reviewed-by: Jinsuk Kim <jinsukkim@chromium.org> > Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org> > Cr-Commit-Position: refs/heads/master@{#614501} TBR=twellington@chromium.org,jinsukkim@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 889682, 907502, 907503, 907770, 907505, 907602 Change-Id: I1cdb497251b4a1f60f955cc6fd90c2be1eae3e96 Reviewed-on: https://chromium-review.googlesource.com/c/1368844 Reviewed-by: Jinsuk Kim <jinsukkim@chromium.org> Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org> Cr-Commit-Position: refs/heads/master@{#614875} [modify] https://crrev.com/b643010d742b1323893685b3ded2250d684882c4/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java [modify] https://crrev.com/b643010d742b1323893685b3ded2250d684882c4/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java [modify] https://crrev.com/b643010d742b1323893685b3ded2250d684882c4/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarButtonInProductHelpController.java |
||||
►
Sign in to add a comment |
||||
Comment 1 by jinsuk...@chromium.org
, Oct 2Status: Assigned (was: Untriaged)