Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2518503004:
[TTS] Fix Bar text showing blank after startup. (Closed)
|
Created:
4 years, 1 month ago by Donn Denman Modified:
4 years ago CC:
chromium-reviews, mdjones+watch_chromium.org, donnd+watch_chromium.org, agrieve+watch_chromium.org, Theresa, PEConn Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[TTS] Fix Bar text showing blank after startup.
The Bar has main bar text that can either show the Search Term or the
Context. When the user taps we show the Context first, and the Search
Term is not yet visible, so calculations based on its size are invalid.
We now check to see if the Search Term text is visible before using any
attribute of it.
BUG=658771
Committed: https://crrev.com/aa73340fc20421e8a9816de1033f6c553cb28983
Cr-Commit-Position: refs/heads/master@{#434723}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Simplify to just check opacity of the bar text. No changes from Overlay needed now! #Patch Set 3 : Update comments only. #
Total comments: 4
Patch Set 4 : Responded to Matt's comments, removed some checks that were not needed. #
Messages
Total messages: 36 (21 generated)
Description was changed from ========== [TTS] Fix Bar text showing blank after startup. Contextual Search sets up the Search Context before initializing the Search Term. This means that the plain "bar text" owned by the Overlay Panel has not yet been initialized. This change adds a way for the OverlayPanel to track whether the bar text has been initialized or not. Contextual Search checks if the bar text is set up yet and conditionally uses the Search Context instead. BUG=658771 ========== to ========== [TTS] Fix Bar text showing blank after startup. Contextual Search sets up the Search Context before initializing the Search Term. This means that the plain "bar text" owned by the Overlay Panel has not yet been initialized when the Search Context is first shown. This change adds a way for the OverlayPanel to track whether the bar text has been initialized or not. Contextual Search checks if the bar text is set up yet and conditionally uses the Search Context instead. BUG=658771 ==========
donnd@chromium.org changed reviewers: + mdjones@chromium.org
Matt, PTAL. Thanks!
All, another simpler approach to consider would be to just initialize the main Bar Text before showing the Context if the Bar text has never been shown. I think this is less robust, and something of a hack, but it is simpler.
If you want to keep the current approach, it looks like you could do it without modifying overlay_panel_layer. All you should need to do is check "search_term_opacity != 0" anywhere you call "HasBarText". I think we should keep the logic inside of overlay_panel_layer as simple as possible; HasBarText is confusing because it doesn't really indicate that the bar has text, just whether or not it is visible. https://codereview.chromium.org/2518503004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchBarControl.java (right): https://codereview.chromium.org/2518503004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchBarControl.java:65: * The opacity of the Bar's Search Context. May not be initialized until opacity beyond 0. "This text control may not be init..." It might be worth also mentioning this in either the constructor or init function if this class has one.
Description was changed from ========== [TTS] Fix Bar text showing blank after startup. Contextual Search sets up the Search Context before initializing the Search Term. This means that the plain "bar text" owned by the Overlay Panel has not yet been initialized when the Search Context is first shown. This change adds a way for the OverlayPanel to track whether the bar text has been initialized or not. Contextual Search checks if the bar text is set up yet and conditionally uses the Search Context instead. BUG=658771 ========== to ========== [TTS] Fix Bar text showing blank after startup. The Bar has main bar text can show either the Search Term or the Context. When the user taps we show the Context first, and the Search Term has not yet been initialized. This causes computations to fail so nothing is shown. We now check to see if the Search Term text has been initialized before using any attribute of it. BUG=658771 ==========
Matt, PTAL. Thanks for the great suggestion to just check opacity each time -- for some reason I thought we'd need to measure the bar text even when not visible.
The CQ bit was checked by donnd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by donnd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/2518503004/diff/40001/chrome/browser/android/... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right): https://codereview.chromium.org/2518503004/diff/40001/chrome/browser/android/... chrome/browser/android/compositor/layer/contextual_search_layer.cc:244: SetupTextLayer(search_bar_top, search_bar_height, Unless there was a reason not to, could we leave this as the expanded version? It's easier to read. https://codereview.chromium.org/2518503004/diff/40001/chrome/browser/android/... chrome/browser/android/compositor/layer/contextual_search_layer.cc:628: if (has_bar_text) The bar_text_ object always exists since it is created in the constructor. Do these "if (has_bar_text_)" checks have any visible impact?
Description was changed from ========== [TTS] Fix Bar text showing blank after startup. The Bar has main bar text can show either the Search Term or the Context. When the user taps we show the Context first, and the Search Term has not yet been initialized. This causes computations to fail so nothing is shown. We now check to see if the Search Term text has been initialized before using any attribute of it. BUG=658771 ========== to ========== [TTS] Fix Bar text showing blank after startup. The Bar has main bar text that can either show the Search Term or the Context. When the user taps we show the Context first, and the Search Term is not yet visible, so calculations based on its size are invalid. We now check to see if the Search Term text is visible before using any attribute of it. BUG=658771 ==========
Matt, PTAL. https://codereview.chromium.org/2518503004/diff/40001/chrome/browser/android/... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right): https://codereview.chromium.org/2518503004/diff/40001/chrome/browser/android/... chrome/browser/android/compositor/layer/contextual_search_layer.cc:244: SetupTextLayer(search_bar_top, search_bar_height, On 2016/11/22 16:37:49, mdjones wrote: > Unless there was a reason not to, could we leave this as the expanded version? > It's easier to read. Done. I agree with you, but git CL format liked this better for some reason. https://codereview.chromium.org/2518503004/diff/40001/chrome/browser/android/... chrome/browser/android/compositor/layer/contextual_search_layer.cc:628: if (has_bar_text) On 2016/11/22 16:37:49, mdjones wrote: > The bar_text_ object always exists since it is created in the constructor. Do > these "if (has_bar_text_)" checks have any visible impact? You're right, the checks earlier are the only ones that are needed. Renamed to bar_text_visible, and removed the check here and the ones below.
The CQ bit was checked by donnd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
twellington@chromium.org changed reviewers: + twellington@chromium.org
lgtm
The CQ bit was checked by donnd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by donnd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480366058222240,
"parent_rev": "6b1bda224de5732daa402d6de222bad096c6e530", "commit_rev":
"94a774b15837c6686dd29d4861d0918129e8c188"}
Message was sent while issue was closed.
Description was changed from ========== [TTS] Fix Bar text showing blank after startup. The Bar has main bar text that can either show the Search Term or the Context. When the user taps we show the Context first, and the Search Term is not yet visible, so calculations based on its size are invalid. We now check to see if the Search Term text is visible before using any attribute of it. BUG=658771 ========== to ========== [TTS] Fix Bar text showing blank after startup. The Bar has main bar text that can either show the Search Term or the Context. When the user taps we show the Context first, and the Search Term is not yet visible, so calculations based on its size are invalid. We now check to see if the Search Term text is visible before using any attribute of it. BUG=658771 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [TTS] Fix Bar text showing blank after startup. The Bar has main bar text that can either show the Search Term or the Context. When the user taps we show the Context first, and the Search Term is not yet visible, so calculations based on its size are invalid. We now check to see if the Search Term text is visible before using any attribute of it. BUG=658771 ========== to ========== [TTS] Fix Bar text showing blank after startup. The Bar has main bar text that can either show the Search Term or the Context. When the user taps we show the Context first, and the Search Term is not yet visible, so calculations based on its size are invalid. We now check to see if the Search Term text is visible before using any attribute of it. BUG=658771 Committed: https://crrev.com/aa73340fc20421e8a9816de1033f6c553cb28983 Cr-Commit-Position: refs/heads/master@{#434723} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/aa73340fc20421e8a9816de1033f6c553cb28983 Cr-Commit-Position: refs/heads/master@{#434723} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
