New issue
Advanced search Search tips

Issue 889682 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Consider moving IPH from Tab to feature-specific classes

Project Member Reported by robertogden@chromium.org, Sep 27

Issue description

This 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.
 
Cc: tedc...@chromium.org
Status: Assigned (was: Untriaged)
Cc: twelling...@chromium.org mdjones@chromium.org
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.
Working on it now.. looks like TabObserver should be enough for this.


Project Member

Comment 4 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Project Member

Comment 7 by bugdroid1@chromium.org, 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

Status: Assigned (was: Fixed)
Summary: Consider moving IPH from Tab to feature-specific classes (was: Consider moving Tab.java didFinishPageLoad IPH and event code to a Tab User Data)
Repurposed this bug for tracking the refactoring effort around IPH.
Project Member

Comment 9 by bugdroid1@chromium.org, 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

Project Member

Comment 10 by bugdroid1@chromium.org, 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